Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

PB-1212: Allow delete last point by button when on line editing mode. #1138

Merged
merged 8 commits into from
Dec 9, 2024
Merged

PB-1212: Allow delete last point by button when on line editing mode. #1138

merged 8 commits into from
Dec 9, 2024

Conversation

ismailsunni
Copy link
Contributor

@ismailsunni ismailsunni commented Nov 26, 2024 •
edited by github-actions bot
Loading

@ismailsunni ismailsunni requested review from ltshb, pakb and ltkum and removed request for ltshb November 26, 2024 10:38
@cypress Cypress
Copy link

cypress bot commented Nov 26, 2024 •
edited
Loading

web-mapviewer    Run #4035

Run Properties:  status check passed Passed #4035  •  git commit 17039868e4: PB-1212: handle delete last point on one place to avoid deleting point twice.
Project web-mapviewer
Branch Review pb-1212-allow-delete-last-point-on-editing-mode
Run status status check passed Passed #4035
Run duration 04m 44s
Commit git commit 17039868e4: PB-1212: handle delete last point on one place to avoid deleting point twice.
Committer Ismail Sunni
View all properties for this run ↗︎
Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 21
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 215
View all changes introduced in this branch ↗︎

pakb
pakb reviewed Nov 26, 2024
View reviewed changes
src/modules/drawing/components/DrawingToolbox.vue Outdated Show resolved Hide resolved
src/modules/drawing/components/DrawingToolbox.vue Outdated
const selectedEditableFeatures = computed(() => store.state.features.selectedEditableFeatures)
const selectedLineString = computed(() => {
if (selectedEditableFeatures.value && selectedEditableFeatures.value.length > 0) {
const selectedFeature = selectedEditableFeatures.value[0]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can there be more than one feature selected while drawing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I know, no. But the store as a list of editable feature. Perhaps there is a way to do it or some historical reason?
I always select the first item. From the user point of view, it will be the same, since there will be always only one item in the list.

src/modules/drawing/components/DrawingToolbox.vue Outdated Show resolved Hide resolved
pakb
pakb reviewed Nov 26, 2024
View reviewed changes
Copy link
Contributor

@pakb pakb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when finishing editing/adding more point to a line measure, the last point is sometimes automatically removed, there might be some call to the remove function where it shouldn't be

@ismailsunni ismailsunni force-pushed the pb-1212-allow-delete-last-point-on-editing-mode branch 2 times, most recently from 8f34164 to 1545f14 Compare December 3, 2024 09:15
@ismailsunni
Copy link
Contributor Author

@pakb I have fixed the automatically removed, but I am not sure this is the same one as yours. Could you check it again?

@ismailsunni ismailsunni requested a review from pakb December 3, 2024 11:45
pakb
pakb reviewed Dec 6, 2024
View reviewed changes
Copy link
Contributor

@pakb pakb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using "right click" works as expected, but using the "Delete last point" button not.

Could you make sure both are using the same logic? And maybe add some tests that uses this button alongside right click, so that we make sure it is working in both cases.

Side question : is it a big effort to make it possible to close the line as a polygon while adding points?
Underlying question is : can we gather the code that manages all "interactions" with a line/measure, and re-use them in both the "add new" and "edit" mode? It looks like the code isn't currently the same

@ismailsunni ismailsunni force-pushed the pb-1212-allow-delete-last-point-on-editing-mode branch from 74c86a2 to a7009ca Compare December 9, 2024 07:48
@ismailsunni
Copy link
Contributor Author

hi @pakb

Using "right click" works as expected, but using the "Delete last point" button not.

Could you make sure both are using the same logic? And maybe add some tests that uses this button alongside right click, so that we make sure it is working in both cases.

Fixed. Both are handled in one place, just different interaction to delete.

Side question : is it a big effort to make it possible to close the line as a polygon while adding points?
Underlying question is : can we gather the code that manages all "interactions" with a line/measure, and re-use them in both the "add new" and "edit" mode? It looks like the code isn't currently the same

I think so, while there are some common features for both Draw Interactions, there are also some different, for example add -> add drawing feature while edit -> change geometry only. Actually, that's my first approach to implement the feature. But it was before I have a better understanding of the geoadmin drawing library. Perhaps with careful state tracker (like I did with EditMode), we can unify the drawing interaction.

pakb
pakb approved these changes Dec 9, 2024
View reviewed changes
Copy link
Contributor

@pakb pakb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, LGTM!
I created another ticket/task to fix the "close as a polygon" issue when adding more points
https://jira.swisstopo.ch/browse/PB-1275

@ismailsunni ismailsunni force-pushed the pb-1212-allow-delete-last-point-on-editing-mode branch from a7009ca to 1703986 Compare December 9, 2024 09:22
@ismailsunni ismailsunni merged commit 17bebc7 into develop Dec 9, 2024
6 checks passed
@ismailsunni ismailsunni deleted the pb-1212-allow-delete-last-point-on-editing-mode branch December 9, 2024 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pakb pakb pakb approved these changes

@ltkum ltkum Awaiting requested review from ltkum

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ismailsunni @pakb